Correct AuthorizationError field declarations for ES 2022 #24405
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where class fields from the super constructor are unintentionally overridden when targeting ES 2022, by updating the property declarations in AuthorizationError.
- Replaces standard field declarations with TypeScript's "declare" syntax for claims and tenantId.
- Adds comments explaining the change to prevent field overriding.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| // When targeting ES 2022 or later, TypeScript generates ES6 class fields for these properties. | ||
| // This overrides the own properties dynamically created by the super constructor. |
There was a problem hiding this comment.
typo - perhaps you mean to omit "own"
But might warrant some rewording. I think you mean to say that readonly claims?: string would override super class properties. It is hard to understand because now it says declare ... which does not have that behvior.
Maybe something like:
When targeting ES 2022 or later, TypeScript generates ES6 class fields for properties listed without declare keyword and overrides the properties dynamically created by the super constructor.
Or just leave it out and amend the following to say:
To prevent undesired overriding of super class properties (when targeting ES 2022 or later),
There was a problem hiding this comment.
I specifically mean "own properties" (the same "own properties" in APIs like "Object.getOwnPropertyNames").
I specifically mentioned this, since overriding inherited properties is much more common and easier, but not what is being done here.
How about:
| // When targeting ES 2022 or later, TypeScript generates ES6 class fields for these properties. | |
| // This overrides the own properties dynamically created by the super constructor. | |
| // When targeting ES 2022 or later, TypeScript would generate ES6 class fields for these properties if they did not use "declare". | |
| // That would override the own properties dynamically created in the super constructor | |
| // resulting in these properties always holding `undefined` instead of their desired values. |
There was a problem hiding this comment.
I think that is more clear: Does that address your concerns?
There was a problem hiding this comment.
The revision works. The difficulty is just English. "the own properties" looks like a "their own properties" because "their own" is an extremely common phrase. Putting "own" in quotes would have helped make it look intentional.
## Description Ensure these tests would fail if error was not thrown, make a bit more concise. Cleanup done while working on #24405
Description
Fix
AuthorizationError.claimsandAuthorizationError.tenantIdso they don't shadow the actual values from the super constructor that are manually copied onto thethisobject when TypeScript introduces an ES6 class fields by targeting ES 2022.Issue detected by #24324
Reviewer Guidance
The review process is outlined on this wiki page.